Skip to content

Conversation

@alexshtin
Copy link
Contributor

What changed?
Renamed:

  • WorkflowTaskTimeout -> DefaultWorkflowTaskTimeout
  • CurrentWorkflowTaskTimeout -> WorkflowTaskTimeout

Why?
For consistency.

How did you test it?
Run tests.

Potential risks
No risks.

@alexshtin alexshtin requested review from mfateev and samarabbas July 17, 2020 01:04
WorkflowExecutionTimeoutSeconds: executionInfo.WorkflowExecutionTimeout,
WorkflowRunTimeoutSeconds: executionInfo.WorkflowRunTimeout,
WorkflowTaskTimeoutSeconds: executionInfo.WorkflowTaskTimeout,
WorkflowTaskTimeoutSeconds: executionInfo.DefaultWorkflowTaskTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename the DescribeWorkflowExecutionResponse to also call it DefaultWorkflowTaskTimeoutSeconds instead of WorkflowTaskTimeoutSeconds to stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are bunch of WorkflowTaskTimeoutSeconds fields in proto. Let's discuss them separately since those changes should be started from api repo anyway.

Copy link
Contributor

@samarabbas samarabbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One minor change and then you can land this.

@alexshtin alexshtin force-pushed the feature/rename-current-workflow-task-timeout branch from 4ef9727 to 818d934 Compare July 20, 2020 15:54
@alexshtin alexshtin merged commit d066a7d into temporalio:master Jul 20, 2020
@alexshtin alexshtin deleted the feature/rename-current-workflow-task-timeout branch July 20, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants